frontend/vitest.setup.ts (1)

45-46: Simplify redundant ternary expression.

The expression query === '(prefers-color-scheme: dark)' ? false : false always returns false. If you want to simulate system preference for dark mode, use a simple boolean or make the condition meaningful.

🔎 Proposed fix

-vi.stubGlobal('matchMedia', vi.fn().mockImplementation((query: string) => ({
-  matches: query === '(prefers-color-scheme: dark)' ? false : false,
+vi.stubGlobal('matchMedia', vi.fn().mockImplementation((query: string) => ({
+  matches: false,

Or if you want to support dark mode testing:

-vi.stubGlobal('matchMedia', vi.fn().mockImplementation((query: string) => ({
-  matches: query === '(prefers-color-scheme: dark)' ? false : false,
+vi.stubGlobal('matchMedia', vi.fn().mockImplementation((query: string) => ({
+  matches: query === '(prefers-color-scheme: dark)',

frontend/playwright.config.ts (1)

16-21: Consider adding more browser projects for broader coverage.

Currently only Chromium is configured. For production-grade E2E testing, consider adding Firefox and WebKit to catch browser-specific issues.

🔎 Suggested addition

   projects: [
     {
       name: 'chromium',
       use: { ...devices['Desktop Chrome'] },
     },
+    {
+      name: 'firefox',
+      use: { ...devices['Desktop Firefox'] },
+    },
+    {
+      name: 'webkit',
+      use: { ...devices['Desktop Safari'] },
+    },
   ],

frontend/e2e/theme.spec.ts (1)

32-60: Tests bypass actual theme-switching mechanism.

These tests manually manipulate localStorage and DOM classes via page.evaluate() instead of using the application's actual theme controls (buttons/selectors). This doesn't validate the real user flow.

Consider clicking actual theme toggle buttons:

test('applies dark theme when set', async ({ page }) => {
  await page.goto('/login');
  
  // Find and click the dark theme button/toggle
  await page.click('[data-testid="theme-toggle"]'); // or appropriate selector
  await page.click('[data-testid="theme-dark"]');
  
  const hasDarkClass = await page.evaluate(() =>
    document.documentElement.classList.contains('dark')
  );
  expect(hasDarkClass).toBe(true);
  
  const storedTheme = await page.evaluate(() => localStorage.getItem('app-theme'));
  expect(storedTheme).toBe('dark');
});

This approach validates the actual theme-switching logic and user interaction.

frontend/e2e/auth.spec.ts (2)

37-47: Document test credentials for environment setup.

The tests use hardcoded credentials (user/user123). Ensure these test users are documented in the project's testing setup guide, or consider using environment variables for flexibility across different testing environments.

Example using environment variables:

const TEST_USERNAME = process.env.TEST_USERNAME || 'user';
const TEST_PASSWORD = process.env.TEST_PASSWORD || 'user123';

---

114-123: Fragile selector for logout button.

The logout button selector uses a broad text-matching approach with multiple fallbacks. This is fragile and may break if button text changes or if multiple elements match.

🔎 Suggested improvement

Add a data-testid to the logout button in your component:

<button data-testid="logout-button" on:click={logout}>Logout</button>

Then simplify the test:

-    const logoutButton = page.locator('button:has-text("Logout"), a:has-text("Logout"), [data-testid="logout"]');
+    const logoutButton = page.locator('[data-testid="logout-button"]');

This makes tests more maintainable and resistant to UI text changes.

.github/workflows/frontend-tests.yml (2)

36-40: Consider removing the redundant test run.

npm run test:coverage already runs all tests with coverage enabled. Running npm test first (line 37) and then npm run test:coverage (line 40) duplicates test execution, increasing CI time without benefit.

🔎 Proposed fix

      - name: Install dependencies
        run: npm ci

-     - name: Run unit tests
-       run: npm test
-
      - name: Run tests with coverage
        run: npm run test:coverage

---

59-62: Pin the yq version for reproducible builds.

Downloading from /latest/ could introduce unexpected behavior if a breaking change is released. Pin to a specific version for CI stability.

🔎 Proposed fix

      - name: Install yq
        run: |
-         sudo wget https://github.com/mikefarah/yq/releases/latest/download/yq_linux_amd64 -O /usr/local/bin/yq
+         sudo wget https://github.com/mikefarah/yq/releases/download/v4.44.3/yq_linux_amd64 -O /usr/local/bin/yq
          sudo chmod +x /usr/local/bin/yq

frontend/src/stores/__tests__/theme.test.ts (1)

150-157: Test assertion doesn't verify "without triggering server save".

The test description states it "updates store without triggering server save", but there's no assertion verifying that saveThemeSetting was not called. Consider adding this verification for completeness.

🔎 Proposed fix

  describe('setThemeLocal', () => {
    it('updates store without triggering server save', async () => {
+     const { saveThemeSetting } = await import('../../lib/user-settings');
      const { theme, setThemeLocal } = await import('../theme');

      setThemeLocal('dark');
      expect(get(theme)).toBe('dark');
      expect(localStorage.setItem).toHaveBeenCalledWith('app-theme', 'dark');
+     expect(saveThemeSetting).not.toHaveBeenCalled();
    });

Configuration used: defaults

Review profile: CHILL

Plan: Pro

frontend/playwright.config.ts (1)

16-21: Consider expanding browser coverage.

Currently only testing with Chromium. For more comprehensive cross-browser testing, consider adding Firefox and WebKit projects.

🔎 Example multi-browser configuration

 projects: [
   {
     name: 'chromium',
     use: { ...devices['Desktop Chrome'] },
   },
+  {
+    name: 'firefox',
+    use: { ...devices['Desktop Firefox'] },
+  },
+  {
+    name: 'webkit',
+    use: { ...devices['Desktop Safari'] },
+  },
 ],

frontend/vitest.setup.ts (1)

45-46: Simplify redundant ternary expression.

The ternary operator returns false for both branches, making it unnecessarily complex.

🔎 Proposed simplification

-  matches: query === '(prefers-color-scheme: dark)' ? false : false,
+  matches: false,

frontend/e2e/auth.spec.ts (2)

37-47: Verify test credentials match environment.

The tests use hardcoded credentials user/user123. Ensure these test credentials are properly configured in your test environment and documented for other developers.

Consider extracting credentials to environment variables or a test configuration file:

const TEST_USERNAME = process.env.TEST_USERNAME || 'user';
const TEST_PASSWORD = process.env.TEST_PASSWORD || 'user123';

---

114-123: Consider using data-testid for more reliable selection.

The logout button selector uses multiple fallback strategies which is flexible but could be brittle. Consider adding a data-testid attribute to the logout button for more reliable testing.

🔎 Recommended approach

In your component:

<button data-testid="logout">Logout</button>

In the test:

const logoutButton = page.locator('[data-testid="logout"]');

frontend/src/stores/__tests__/auth.test.ts (3)

36-42: Consider removing redundant vi.clearAllMocks() in afterEach.

vi.clearAllMocks() in afterEach (Line 42) is redundant since mockXxx.mockReset() calls in beforeEach already clear the mocks before each test. Additionally, vi.resetModules() provides further isolation.

🔎 Proposed simplification

-  afterEach(() => {
-    vi.clearAllMocks();
-  });

---

149-158: Consider asserting specific error content.

The test uses .rejects.toBeDefined() which only verifies an error was thrown, but doesn't validate the error message or type. This could mask unexpected error conditions.

🔎 Proposed improvement

-      await expect(login('baduser', 'badpass')).rejects.toBeDefined();
+      await expect(login('baduser', 'badpass')).rejects.toThrow('Invalid credentials');

---

343-352: Same improvement opportunity for error assertion.

Similar to the login test, consider asserting specific error content rather than just checking the error is defined.

🔎 Proposed improvement

-      await expect(fetchUserProfile()).rejects.toBeDefined();
+      await expect(fetchUserProfile()).rejects.toThrow('Unauthorized');

frontend/e2e/theme.spec.ts (1)

62-74: Persistence test only verifies localStorage, not DOM state.

This test confirms localStorage persists across navigation (which is expected browser behavior), but doesn't verify that the theme is actually applied after navigation. Consider also checking the DOM class state on /register.

🔎 Proposed enhancement

   test('theme persists across page navigation', async ({ page }) => {
     await page.goto('/login');
 
     // Set dark theme
     await page.evaluate(() => localStorage.setItem('app-theme', 'dark'));
     await page.reload();
 
     // Navigate to another page
     await page.goto('/register');
 
     const storedTheme = await page.evaluate(() => localStorage.getItem('app-theme'));
     expect(storedTheme).toBe('dark');
+
+    // Also verify the theme is applied on the new page
+    const hasDarkClass = await page.evaluate(() =>
+      document.documentElement.classList.contains('dark')
+    );
+    expect(hasDarkClass).toBe(true);
   });

.github/workflows/frontend-tests.yml (4)

36-40: Running tests twice is redundant.

Lines 37 and 40 run tests separately (npm test then npm run test:coverage). Coverage runs typically execute the same tests with instrumentation. Consider running only the coverage command.

🔎 Proposed optimization

-      - name: Run unit tests
-        run: npm test
-
       - name: Run tests with coverage
         run: npm run test:coverage

---

59-62: Pin yq version for reproducible builds.

Downloading from /releases/latest/ can cause unexpected behavior when a new version is released. Pin to a specific version for reproducibility.

🔎 Proposed fix

       - name: Install yq
         run: |
-          sudo wget https://github.com/mikefarah/yq/releases/latest/download/yq_linux_amd64 -O /usr/local/bin/yq
+          sudo wget https://github.com/mikefarah/yq/releases/download/v4.44.1/yq_linux_amd64 -O /usr/local/bin/yq
           sudo chmod +x /usr/local/bin/yq

---

166-168: Consider installing all configured browsers or caching.

Installing only chromium is fine if that's the only browser in the Playwright config. However, Playwright browser binaries are large; consider caching them to speed up CI.

🔎 Example with caching

Add before the install step:

      - name: Cache Playwright browsers
        uses: actions/cache@v4
        with:
          path: ~/.cache/ms-playwright
          key: playwright-${{ runner.os }}-${{ hashFiles('frontend/package-lock.json') }}

---

64-70: Pin k3s version for CI stability.

The k3s install script can specify a version using the INSTALL_K3S_VERSION environment variable. Without pinning, the latest tag is not maintained, which can introduce unexpected variations between CI runs. For production workflows, specify a stable version:

       - name: Setup Kubernetes (k3s)
         run: |
-          curl -sfL https://get.k3s.io | INSTALL_K3S_EXEC="--disable=traefik --tls-san host.docker.internal" sh -
+          curl -sfL https://get.k3s.io | INSTALL_K3S_VERSION="v1.33.4+k3s1" INSTALL_K3S_EXEC="--disable=traefik --tls-san host.docker.internal" sh -

Configuration used: defaults

Review profile: CHILL

Plan: Pro

.github/workflows/frontend-tests.yml (1)

166-168: Frontend readiness check still masks failures with || true.

As noted in previous reviews, the || true suffix allows the workflow to continue even if the frontend never becomes ready, potentially causing misleading E2E test failures.

.github/workflows/frontend-tests.yml (4)

36-40: Consider removing the redundant test run.

Running npm test followed by npm run test:coverage executes the test suite twice. Since test:coverage typically runs all tests with coverage instrumentation, the separate npm test step appears unnecessary.

🔎 Proposed optimization

      - name: Install dependencies
        run: npm ci

-     - name: Run unit tests
-       run: npm test
-
      - name: Run tests with coverage
        run: npm run test:coverage

---

74-77: Pin the yq version for reproducible builds.

Downloading from /latest/ can introduce unexpected breaking changes if yq releases a new version with different behavior or CLI changes.

🔎 Proposed fix

      - name: Install yq
        run: |
-         sudo wget https://github.com/mikefarah/yq/releases/latest/download/yq_linux_amd64 -O /usr/local/bin/yq
+         sudo wget https://github.com/mikefarah/yq/releases/download/v4.44.6/yq_linux_amd64 -O /usr/local/bin/yq
          sudo chmod +x /usr/local/bin/yq

Check the yq releases page for the current stable version.

---

120-143: Consider a more maintainable approach for CI configuration.

The 24 separate yq commands make this workflow fragile and difficult to maintain. If the base docker-compose.yaml structure changes, these commands may break or produce unexpected results.

Consider one of these alternatives:

1. Maintain a separate docker-compose.ci.yaml in the repository with CI-specific overrides using Docker Compose's native override mechanism:

   # docker-compose.ci.yaml in repo
   services:
     backend:
       environment:
         - TESTING=true
         - MONGO_ROOT_USER=root
         # ... other CI overrides

   Then use: docker compose -f docker-compose.yaml -f docker-compose.ci.yaml up

2. Consolidate yq modifications into a single multi-document operation or shell script in the repo for better version control and testing.

3. Use a yq script file (.yq) committed to the repo that can be tested and reviewed separately.

This would improve maintainability and make CI configuration changes reviewable via PR.

---

183-190: Consider collecting logs from additional services for comprehensive debugging.

Currently, only frontend and backend logs are collected separately. Including logs from supporting services (kafka, zookeeper, mongo, redis) could help diagnose infrastructure-related E2E test failures.

🔎 Enhanced log collection

      - name: Collect logs on failure
        if: failure()
        run: |
          mkdir -p logs
          docker compose -f docker-compose.ci.yaml logs > logs/docker-compose.log
          docker compose -f docker-compose.ci.yaml logs frontend > logs/frontend.log
          docker compose -f docker-compose.ci.yaml logs backend > logs/backend.log
+         docker compose -f docker-compose.ci.yaml logs kafka > logs/kafka.log
+         docker compose -f docker-compose.ci.yaml logs zookeeper > logs/zookeeper.log
+         docker compose -f docker-compose.ci.yaml logs mongo > logs/mongo.log

Configuration used: defaults

Review profile: CHILL

Plan: Pro

.github/workflows/ci.yml (2)

31-37: Redundant test execution.

Running npm test (line 33) followed by npm run test:coverage (line 37) likely executes the test suite twice. If test:coverage runs tests with coverage, consider removing the plain npm test step or combining them.

🔎 Proposed simplification

      - name: Run frontend unit tests
        working-directory: frontend
-       run: npm test
-
-     - name: Run frontend tests with coverage
-       working-directory: frontend
-       run: npm run test:coverage
+       run: npm run test:coverage

---

159-169: Hardcoded credentials in workflow.

The MongoDB credentials (root/rootpassword) are hardcoded. While acceptable for CI-only test databases, consider documenting this or using GitHub Actions variables for consistency. The Checkov hint (CKV_SECRET_4) flags this as a medium-severity finding.

For better maintainability, define these as workflow-level environment variables or use repository variables:

env:
  MONGO_TEST_USER: root
  MONGO_TEST_PASSWORD: rootpassword

.github/actions/setup-ci-compose/action.yml (1)

26-29: Credentials duplicated between action and workflow.

The credentials MONGO_ROOT_USER=root and MONGO_ROOT_PASSWORD=rootpassword are hardcoded here and also in the main workflow (lines 162-165). Consider parameterizing these as action inputs to maintain a single source of truth.

🔎 Example parameterization

Add inputs to the action:

inputs:
  kubeconfig-path:
    description: Path to kubeconfig file for cert-generator mount
    required: true
  mongo-user:
    description: MongoDB root user
    default: 'root'
  mongo-password:
    description: MongoDB root password
    default: 'rootpassword'

Then use ${{ inputs.mongo-user }} and ${{ inputs.mongo-password }} in the yq commands.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

.github/workflows/ci.yml (3)

133-136: Consider removing silent failure for frontend health check.

Past review comment flagged this: The || true causes this step to always succeed even if the frontend never starts. This could lead to confusing E2E test failures.

Consider failing fast or adding explicit logging:

🔎 Alternative approaches

Option 1: Remove || true to fail fast:

-         curl --retry 30 --retry-delay 5 --retry-all-errors -ksf https://127.0.0.1:5001/ || true
+         curl --retry 30 --retry-delay 5 --retry-all-errors -ksf https://127.0.0.1:5001/

Option 2: Add explicit warning:

-         curl --retry 30 --retry-delay 5 --retry-all-errors -ksf https://127.0.0.1:5001/ || true
+         curl --retry 30 --retry-delay 5 --retry-all-errors -ksf https://127.0.0.1:5001/ || echo "::warning::Frontend health check failed"

Based on past review comments.

---

172-182: Fix Codecov action input parameter name.

Past review comment and static analysis both confirm: The codecov action v5 uses files (plural), not file. This will cause the coverage upload to fail.

🔎 Proposed fix

       - name: Upload backend coverage to Codecov
         uses: codecov/codecov-action@v5
         if: always()
         with:
           token: ${{ secrets.CODECOV_TOKEN }}
-          file: backend/coverage.xml
+          files: backend/coverage.xml
           flags: backend
           name: backend-coverage
           slug: HardMax71/Integr8sCode
           fail_ci_if_error: false

Based on past review comments and static analysis (actionlint).

---

56-56: Fix sudo redirect issue.

Past review comment flagged this: sudo doesn't affect the redirect operator (>). The file will be written with the runner's permissions, not elevated privileges. This could cause permission issues.

🔎 Proposed fix using sudo tee

-         sudo k3s kubectl config view --raw > /home/runner/.kube/config
+         sudo k3s kubectl config view --raw | sudo tee /home/runner/.kube/config > /dev/null

Based on past review comments and static analysis (actionlint).

Configuration used: defaults

Review profile: CHILL

Plan: Pro

.github/workflows/ci.yml (2)

78-86: Shell scripting issues remain unaddressed.

Two issues persist from previous reviews:

1. Line 82: sudo doesn't affect redirects—the config file won't be written with proper permissions. Use sudo tee instead.

2. Line 84: export KUBECONFIG in a run step doesn't persist to subsequent steps in GitHub Actions.

🔎 Recommended fix

      - name: Setup Kubernetes (k3s)
        run: |
          curl -sfL https://get.k3s.io | INSTALL_K3S_EXEC="--disable=traefik --tls-san host.docker.internal" sh -
          mkdir -p /home/runner/.kube
-         sudo k3s kubectl config view --raw > /home/runner/.kube/config
+         sudo k3s kubectl config view --raw | sudo tee /home/runner/.kube/config > /dev/null
          sudo chmod 600 /home/runner/.kube/config
-         export KUBECONFIG=/home/runner/.kube/config
          timeout 90 bash -c 'until sudo k3s kubectl cluster-info; do sleep 5; done'

Then add KUBECONFIG to the job-level environment:

  frontend-e2e:
    name: Frontend E2E Tests
    needs: frontend-unit
    runs-on: ubuntu-latest
+   env:
+     KUBECONFIG: /home/runner/.kube/config
    steps:

Based on past review comments.

---

190-197: Same shell scripting issues as frontend-e2e job.

Lines 194 and 196 have the same issues flagged in the frontend-e2e job:

1. sudo redirect doesn't work as expected
2. export KUBECONFIG doesn't persist to subsequent steps

Apply the same fixes as recommended for lines 78-86, and add job-level KUBECONFIG environment variable.

Based on past review comments.

frontend/e2e/auth.spec.ts (1)

55-66: Optional: Strengthen loading state verification.

The regex /Logging in|Sign in/ accepts both the loading and non-loading states, so the test passes whether or not the loading state actually appears. This reduces the test's ability to catch regressions.

Consider checking for the loading state more specifically, or verifying the button is disabled during submission.

🔎 Alternative approach

  test('shows loading state during login', async ({ page }) => {
    await page.goto('/login');
    await page.waitForSelector('#username');

    await page.fill('#username', 'user');
    await page.fill('#password', 'user123');

    const submitButton = page.locator('button[type="submit"]');
+   
+   // Verify button is enabled before click
+   await expect(submitButton).toBeEnabled();
+   
    await submitButton.click();

-   await expect(submitButton).toContainText(/Logging in|Sign in/);
+   // Verify loading state appears (button disabled or shows loading text)
+   await expect(submitButton).toBeDisabled();
+   // Or more specifically check for loading text:
+   // await expect(submitButton).toContainText('Logging in');
  });

Configuration used: defaults

Review profile: CHILL

Plan: Pro

.github/workflows/ci.yml (2)

194-201: Same KUBECONFIG export issue as frontend-e2e job.

The export KUBECONFIG on line 200 has the same limitation as in the frontend-e2e job (line 84). It only persists for the timeout command on line 201.

See the earlier comment on lines 78-86 for the recommended fix.

---

78-86: KUBECONFIG export persists only within the current step.<!-- [scratchpad_start]
[task review_file_3/3 review_k3s_setup_1]
Review k3s setup in frontend-e2e job
[observations]

• Line 84: export KUBECONFIG=/home/runner/.kube/config
• Web search confirms: export in GitHub Actions run step does NOT persist to subsequent steps
• Each step gets its own shell process
• To persist across steps, need to use $GITHUB_ENV or job-level env
  [analysis]
  The export on line 84 only affects the timeout command on line 85 within the same step
  Subsequent steps that might need kubectl won't have KUBECONFIG set
  However, looking at the workflow:
• Line 112: kubeconfig-path is passed to setup-ci-compose action
• No other steps seem to directly use kubectl in frontend-e2e job
• The export is only used for the timeout check on line 85
  Severity: This is technically correct for the timeout command but could cause issues if kubectl is used elsewhere
  Actually, looking more carefully, the export is used in the timeout command on line 85 which is in the same step, so it works for that purpose
  But the step name "Setup Kubernetes (k3s)" suggests this is setting up k8s for later use, which would require KUBECONFIG to persist
  [actions]
  Flag this as a potential issue with request_verification to check if kubectl is used in subsequent steps
  [scratchpad_end] -->

The export KUBECONFIG at line 84 only persists within that step (for the timeout command on line 85). Each run step gets its own shell instance, so exports don't carry over between steps.

However, examining the workflow, I see that:

1. The export is used immediately on line 85 (same step) ✅
2. Line 112 passes the kubeconfig path explicitly to the action ✅
3. No other steps in this job directly invoke kubectl

So the current implementation appears functional, but it's fragile. If future steps need kubectl access, they won't have KUBECONFIG set.

🔎 Recommended fix for robustness

To persist environment variables across steps, write to the $GITHUB_ENV file.

       - name: Setup Kubernetes (k3s)
         run: |
           curl -sfL https://get.k3s.io | INSTALL_K3S_EXEC="--disable=traefik --tls-san host.docker.internal" sh -
           mkdir -p /home/runner/.kube
           sudo k3s kubectl config view --raw > /home/runner/.kube/config
           sudo chmod 600 /home/runner/.kube/config
-          export KUBECONFIG=/home/runner/.kube/config
+          echo "KUBECONFIG=/home/runner/.kube/config" >> $GITHUB_ENV
           timeout 90 bash -c 'until sudo k3s kubectl cluster-info; do sleep 5; done'

Or, set it at job-level if it's constant:

  frontend-e2e:
    name: Frontend E2E Tests
    needs: frontend-unit
    runs-on: ubuntu-latest
    env:
      KUBECONFIG: /home/runner/.kube/config
    steps:
      # ...

frontend/rollup.config.js (1)

65-72: Consider increasing the socket timeout for development.

The 2-second timeout is quite aggressive and may cause issues during debugging or on slower networks. Consider increasing it to 5-10 seconds for a better development experience while still preventing indefinite hangs.

🔎 Suggested adjustment

             proxyReq.on('socket', (socket) => {
-                socket.setTimeout(2000);
+                socket.setTimeout(5000);  // 5s is more forgiving during development
                 socket.on('timeout', () => {
                     console.error('Proxy socket timeout - backend unreachable');
                     proxyReq.destroy(new Error('Socket timeout'));
                 });
             });

frontend/playwright.config.ts (1)

1-34: LGTM! Well-structured Playwright configuration.

The configuration is properly set up with:

• Appropriate timeouts for E2E tests (10s per test, 3s for assertions)
• CI-aware settings (retries, workers, reporters)
• Conditional webServer that starts dev server locally but relies on docker-compose in CI
• HTTPS error handling for self-signed certificates

💡 Optional: Consider adding Firefox for broader coverage

While Chromium coverage is sufficient for most cases, you could optionally add Firefox to catch browser-specific issues:

   projects: [
     {
       name: 'chromium',
       use: { ...devices['Desktop Chrome'] },
     },
+    {
+      name: 'firefox',
+      use: { ...devices['Desktop Firefox'] },
+    },
   ],

This can be deferred if it increases CI time significantly.

.github/workflows/ci.yml (1)

78-85: Consider extracting duplicate k3s setup into a reusable action.

The k3s setup code is duplicated between the frontend-e2e (lines 78-85) and backend-integration (lines 194-201) jobs. Similarly, the kubeconfig creation (lines 87-107 and 203-223) is duplicated.

💡 Refactoring suggestion

Create a composite action .github/actions/setup-k3s/action.yml:

name: 'Setup k3s'
description: 'Install k3s and create kubeconfig'
outputs:
  kubeconfig-path:
    description: 'Path to kubeconfig'
    value: /home/runner/.kube/config
runs:
  using: 'composite'
  steps:
    - name: Setup Kubernetes (k3s)
      shell: bash
      run: |
        curl -sfL https://get.k3s.io | INSTALL_K3S_EXEC="--disable=traefik --tls-san host.docker.internal" sh -
        mkdir -p /home/runner/.kube
        sudo k3s kubectl config view --raw | sudo tee /home/runner/.kube/config > /dev/null
        sudo chmod 600 /home/runner/.kube/config
        echo "KUBECONFIG=/home/runner/.kube/config" >> $GITHUB_ENV
        timeout 90 bash -c 'until sudo k3s kubectl cluster-info; do sleep 5; done'
    
    - name: Create kubeconfig for CI
      shell: bash
      run: |
        cat > backend/kubeconfig.yaml <<EOF
        apiVersion: v1
        kind: Config
        clusters:
        - name: ci-cluster
          cluster:
            server: https://host.docker.internal:6443
            insecure-skip-tls-verify: true
        users:
        - name: ci-user
          user:
            token: "ci-token"
        contexts:
        - name: ci
          context:
            cluster: ci-cluster
            user: ci-user
        current-context: ci
        EOF

Then in both jobs:

      - name: Setup k3s
        uses: ./.github/actions/setup-k3s

This would eliminate ~40 lines of duplicated code and improve maintainability.

Also applies to: 194-201

Configuration used: defaults

Review profile: CHILL

Plan: Pro

.github/workflows/ci.yml (2)

264-276: Review hardcoded credentials flagged by past comments and static analysis.

As noted in previous reviews, the hardcoded MongoDB credentials on lines 268-269 should be moved to GitHub Secrets per security best practices, even though they are ephemeral CI credentials. Additionally, the static analysis tool (Checkov) is flagging lines 272-273 for basic auth credentials in the MongoDB URL.

While these are test credentials scoped to CI, consider using GitHub Secrets for consistency with security best practices. See the previous review comment for the recommended approach using ${{ secrets.MONGO_TEST_USER }} and ${{ secrets.MONGO_TEST_PASSWORD }}.

---

78-86: Address shell quoting and KUBECONFIG persistence issues flagged in past reviews.

Two issues remain from previous review comments:

1. Shell quoting: Variables like $HOME (or the hardcoded path) should be properly quoted, and the sudo redirect issue should be fixed with sudo tee.
2. KUBECONFIG export: The export KUBECONFIG on line 84 won't persist to subsequent steps. This needs to be set at the job level or in each step that requires it.

Based on past review comments, these issues were identified but may not have been fully addressed. Please review the previous suggestions for the complete fix.

frontend/playwright.config.ts (1)

14-19: Consider optimizing trace capture to reduce overhead.

The trace: 'on' setting captures traces for all tests, including successful ones, which can consume significant storage and slow down test execution. Consider using 'retain-on-failure' or 'on-first-retry' to capture traces only when needed for debugging.

🔎 Proposed optimization

  use: {
    baseURL: 'https://localhost:5001',
    ignoreHTTPSErrors: true,
-   trace: 'on',
+   trace: 'retain-on-failure',
    screenshot: 'only-on-failure',
  },

Alternative: Use 'on-first-retry' if you want traces only when tests are retried.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

frontend/src/stores/__tests__/notificationStore.test.ts (1)

1-465: Exemplary test suite with comprehensive coverage.

This test file demonstrates excellent testing practices:

• Well-structured with clear describe blocks grouping related functionality
• Comprehensive coverage of all store methods (load, add, markAsRead, markAllAsRead, delete, clear, refresh)
• Both success and failure paths tested consistently
• State mutations, return values, and API invocations all verified
• Derived stores tested for correctness and reactivity
• Proper test isolation via module resets and dynamic imports
• Good edge case coverage (100-item cap, various parameter combinations)

Optional enhancement: Verify mock API signatures match actual implementation.

While the mocks appear correctly structured, it would be beneficial to verify they match the actual API signatures, especially since this is the first time comprehensive frontend tests are being added.

🔎 Verification script

#!/bin/bash
# Verify that mocked API functions exist in the actual API module

echo "=== Checking API functions referenced in mocks ==="
echo ""

# Search for the API function definitions/exports
echo "1. Checking getNotificationsApiV1NotificationsGet:"
ast-grep --pattern 'export $_ getNotificationsApiV1NotificationsGet'

echo ""
echo "2. Checking markNotificationReadApiV1NotificationsNotificationIdReadPut:"
ast-grep --pattern 'export $_ markNotificationReadApiV1NotificationsNotificationIdReadPut'

echo ""
echo "3. Checking markAllReadApiV1NotificationsMarkAllReadPost:"
ast-grep --pattern 'export $_ markAllReadApiV1NotificationsMarkAllReadPost'

echo ""
echo "4. Checking deleteNotificationApiV1NotificationsNotificationIdDelete:"
ast-grep --pattern 'export $_ deleteNotificationApiV1NotificationsNotificationIdDelete'

echo ""
echo "=== Checking actual API module exports ==="
rg -n "export.*(?:getNotifications|markNotificationRead|markAllRead|deleteNotification)" --type ts frontend/src/lib/

frontend/src/App.svelte (1)

30-32: Type mismatch with store value.

The globalError type is { error: Error | string; title?: string } but appError store emits AppError | null which also includes a timestamp field. While this works because extra properties are ignored, consider using the AppError type directly for consistency.

-    let globalError = $state<{ error: Error | string; title?: string } | null>(null);
+    import type { AppError } from './stores/errorStore';
+    let globalError = $state<AppError | null>(null);

frontend/src/components/ErrorDisplay.svelte (1)

11-17: Consider using SPA navigation for "Go to Home" action.

Using window.location.href = '/' causes a full page reload. If the error is recoverable and doesn't require a full reload, consider using the SPA router's navigation. However, given this is for fatal errors, a full reload may be intentional to reset application state.

frontend/src/stores/errorStore.ts (1)

9-10: Unused update import.

The update function is destructured from writable() but never used. Consider removing it to keep the code clean.

-  const { subscribe, set, update } = writable<AppError | null>(null);
+  const { subscribe, set } = writable<AppError | null>(null);

Configuration used: defaults

Review profile: CHILL

Plan: Pro

.github/actions/setup-ci-compose/action.yml (3)

12-16: Add checksum verification for the yq binary.

Downloading binaries without checksum verification poses a security risk. If the download is compromised, a malicious binary could be executed in the CI environment.

🔎 Proposed fix with SHA-256 verification

    - name: Install yq
      shell: bash
      run: |
-       sudo wget -qO /usr/local/bin/yq https://github.com/mikefarah/yq/releases/download/v4.50.1/yq_linux_amd64
+       sudo wget -qO /usr/local/bin/yq https://github.com/mikefarah/yq/releases/download/v4.50.1/yq_linux_amd64
+       echo "c35f640bf4fa5c4e8c2d8f97d3c057aa8f52c7cfce24f3ccb0c07c6fc3aa8cba  /usr/local/bin/yq" | sha256sum -c -
        sudo chmod +x /usr/local/bin/yq

---

22-23: Add validation for docker-compose.yaml existence.

If docker-compose.yaml does not exist, the cp command will fail, and subsequent yq commands will produce unclear error messages.

🔎 Proposed fix

      run: |
+       if [ ! -f docker-compose.yaml ]; then
+         echo "Error: docker-compose.yaml not found"
+         exit 1
+       fi
        cp docker-compose.yaml docker-compose.ci.yaml

---

26-29: Consider consolidating multiple yq calls for efficiency.

Four separate yq eval calls are used to add backend environment variables. While functionally correct, this can be consolidated into a single call for better efficiency.

🔎 Proposed consolidation

-       # Backend environment variables
-       yq eval '.services.backend.environment += ["TESTING=true"]' -i docker-compose.ci.yaml
-       yq eval '.services.backend.environment += ["MONGO_ROOT_USER=root"]' -i docker-compose.ci.yaml
-       yq eval '.services.backend.environment += ["MONGO_ROOT_PASSWORD=rootpassword"]' -i docker-compose.ci.yaml
-       yq eval '.services.backend.environment += ["OTEL_SDK_DISABLED=true"]' -i docker-compose.ci.yaml
+       # Backend environment variables
+       yq eval '.services.backend.environment += ["TESTING=true", "MONGO_ROOT_USER=root", "MONGO_ROOT_PASSWORD=rootpassword", "OTEL_SDK_DISABLED=true"]' -i docker-compose.ci.yaml

Configuration used: defaults

Review profile: CHILL

Plan: Pro

backend/workers/dlq_processor.py (1)

104-104: Verify test database isolation (same concern as other workers).

This worker now uses settings.DATABASE_NAME unconditionally. Ensure test configurations override this setting appropriately to avoid writing to production databases.

backend/workers/run_saga_orchestrator.py (1)

34-34: Verify test database isolation (same concern as other workers).

This worker now uses settings.DATABASE_NAME unconditionally. Ensure test configurations override this setting appropriately.

backend/app/services/coordinator/coordinator.py (1)

557-557: Verify test database isolation in coordinator service.

The coordinator now uses settings.DATABASE_NAME unconditionally. Since this is a critical service handling execution scheduling, it's especially important to ensure test configurations properly isolate the test database.

backend/app/settings.py (1)

12-12: Consider adding test database safeguards.

The new DATABASE_NAME field centralizes database configuration, which improves consistency. However, consider adding validation to prevent accidental production database usage in test environments.

💡 Suggested safeguard implementation

Add a validator to ensure test safety:

from pydantic import field_validator

class Settings(BaseSettings):
    PROJECT_NAME: str = "integr8scode"
    DATABASE_NAME: str = "integr8scode_db"
    # ... other fields ...
    
    TESTING: bool = False
    
    @field_validator('DATABASE_NAME')
    @classmethod
    def validate_test_database(cls, v: str, info) -> str:
        """Warn if TESTING=True but database name doesn't indicate test database"""
        testing = info.data.get('TESTING', False)
        if testing and 'test' not in v.lower():
            import logging
            logging.warning(
                f"TESTING=True but DATABASE_NAME='{v}' doesn't contain 'test'. "
                f"Ensure you're not using a production database in tests."
            )
        return v

Note: Since TESTING is defined after DATABASE_NAME in the class, you may need to adjust field ordering or validation logic.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

backend/tests/conftest.py (1)

93-94: Consider making the test database name prefix configurable.

The current implementation hardcodes the "integr8scode_test" prefix when generating unique test database names. For better flexibility and maintainability, consider deriving the base name from the environment (e.g., reading a base DATABASE_NAME from .env.test) and appending the session/worker IDs.

🔎 Proposed refactor to make prefix configurable

    # Unique database name for test isolation
-   os.environ["DATABASE_NAME"] = f"integr8scode_test_{session_id}_{worker_id}"
+   base_db_name = os.environ.get("DATABASE_NAME", "integr8scode_test")
+   # Strip any existing session/worker suffix to get clean base name
+   base_clean = base_db_name.split("_test_")[0] if "_test_" in base_db_name else base_db_name
+   os.environ["DATABASE_NAME"] = f"{base_clean}_test_{session_id}_{worker_id}"

This allows .env.test to control the base database name while still ensuring unique isolation per test session/worker.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

backend/.env (1)

1-2: Reorder keys alphabetically for consistency.

The dotenv-linter tool suggests placing DATABASE_NAME before PROJECT_NAME to maintain alphabetical ordering across the file. While not critical, this improves consistency and readability.

🔎 Proposed fix

-PROJECT_NAME=integr8scode
 DATABASE_NAME=integr8scode_db
+PROJECT_NAME=integr8scode

Configuration used: defaults

Review profile: CHILL

Plan: Pro

.github/workflows/frontend-ci.yml (3)

75-109: Consider extracting shared k3s setup to a reusable workflow or composite action.

The k3s cluster setup (lines 75-82), kubeconfig creation (lines 84-104), and CI compose setup (lines 106-109) are nearly identical to backend-ci.yml. Extracting this to a composite action would reduce duplication and simplify maintenance.

This is acceptable as-is if you prefer keeping workflows self-contained and independent.

---

141-145: Consider adding a timeout for E2E tests.

Unlike the backend integration tests (which have timeout-minutes: 5), this step has no timeout. E2E tests can hang indefinitely if something goes wrong. Consider adding a timeout to prevent runaway jobs.

🔎 Proposed fix

       - name: Run E2E tests
         working-directory: frontend
+        timeout-minutes: 10
         env:
           CI: true
         run: npx playwright test --reporter=html

---

154-160: Consider collecting Kubernetes logs on failure.

The backend workflow collects k8s events and pod descriptions on failure (lines 133-134 in backend-ci.yml). Since the E2E job also uses k3s, these logs could be valuable for debugging.

🔎 Proposed addition

       - name: Collect logs
         if: failure()
         run: |
           mkdir -p logs
           docker compose -f docker-compose.ci.yaml logs > logs/docker-compose.log
           docker compose -f docker-compose.ci.yaml logs backend > logs/backend.log
           docker compose -f docker-compose.ci.yaml logs frontend > logs/frontend.log
+          kubectl get events --sort-by='.metadata.creationTimestamp' > logs/k8s-events.log 2>&1 || true
+          kubectl describe pods -A > logs/k8s-describe-pods.log 2>&1 || true

Configuration used: defaults

Review profile: CHILL

Plan: Pro

.github/workflows/backend-ci.yml (1)

88-100: Consider using repository secrets or variables for CI credentials.

The hardcoded MongoDB credentials (root/rootpassword) triggered a static analysis warning (CKV_SECRET_4). While acceptable for ephemeral CI environments, using GitHub Actions secrets or variables would:

• Silence security scanners
• Make credential rotation easier
• Follow the principle of least privilege

This is a minor improvement and not blocking since these are clearly test-only credentials for an isolated CI environment.

🔎 Suggested approach

      - name: Run integration tests
        timeout-minutes: 5
        env:
          BACKEND_BASE_URL: https://127.0.0.1:443
-          MONGO_ROOT_USER: root
-          MONGO_ROOT_PASSWORD: rootpassword
+          MONGO_ROOT_USER: ${{ vars.CI_MONGO_USER || 'root' }}
+          MONGO_ROOT_PASSWORD: ${{ secrets.CI_MONGO_PASSWORD || 'rootpassword' }}
          MONGODB_HOST: 127.0.0.1
          MONGODB_PORT: 27017
-          MONGODB_URL: mongodb://root:[email protected]:27017/?authSource=admin
+          MONGODB_URL: mongodb://${{ vars.CI_MONGO_USER || 'root' }}:${{ secrets.CI_MONGO_PASSWORD || 'rootpassword' }}@127.0.0.1:27017/?authSource=admin

Configuration used: defaults

Review profile: CHILL

Plan: Pro

.github/workflows/backend-ci.yml (1)

88-100: Consider using secrets for test credentials (optional).

The hardcoded MongoDB credentials in the environment variables are flagged by static analysis (CKV_SECRET_4). While these are CI-only test credentials with no production impact, using GitHub secrets would be slightly more hygienic and prevent credential patterns from being copied elsewhere.

Since this is an ephemeral CI environment, the security risk is minimal, so this is purely a stylistic preference.

🔎 Optional: Use secrets for credentials

       env:
         BACKEND_BASE_URL: https://127.0.0.1:443
-        MONGO_ROOT_USER: root
-        MONGO_ROOT_PASSWORD: rootpassword
+        MONGO_ROOT_USER: ${{ secrets.CI_MONGO_USER || 'root' }}
+        MONGO_ROOT_PASSWORD: ${{ secrets.CI_MONGO_PASSWORD || 'rootpassword' }}
         MONGODB_HOST: 127.0.0.1
         MONGODB_PORT: 27017
-        MONGODB_URL: mongodb://root:[email protected]:27017/?authSource=admin
+        MONGODB_URL: mongodb://${{ secrets.CI_MONGO_USER || 'root' }}:${{ secrets.CI_MONGO_PASSWORD || 'rootpassword' }}@127.0.0.1:27017/?authSource=admin

Configuration used: defaults

Review profile: CHILL

Plan: Pro

frontend/src/stores/__tests__/errorStore.test.ts (2)

16-82: Excellent coverage of setError scenarios.

The tests thoroughly verify Error objects, strings, titles, logging, timestamps, and overwriting behavior.

Optional: Consider adding test for Error object with title

For completeness, you could add a test case combining an Error object with a custom title:

+    it('sets Error object with title', () => {
+      const error = new Error('Error message');
+      appError.setError(error, 'Custom Title');
+
+      const current = get(appError);
+      expect(current).not.toBe(null);
+      expect(current?.error).toBe(error);
+      expect(current?.title).toBe('Custom Title');
+    });
+
     it('logs error to console', () => {

---

99-118: LGTM! Subscription behavior thoroughly verified.

The test correctly validates the subscription mechanism, including the initial value, subsequent updates, clearing, and unsubscribing.

Optional: Consider simplifying the type on Line 101

The conditional type inference is correct but verbose. If the AppError interface is exported from errorStore, you could simplify:

-      const values: (typeof appError extends { subscribe: (fn: (v: infer T) => void) => void } ? T : never)[] = [];
+      const values: (AppError | null)[] = [];

This would require adding to your imports:

-import { appError } from '../errorStore';
+import { appError, type AppError } from '../errorStore';

frontend/src/utils/__tests__/meta.test.ts (1)

98-105: Consider expanding empty string test coverage.

The current test verifies that an empty string doesn't update the title, but doesn't check description behavior. Consider also verifying:

1. That no meta description tag is created when an empty description string is passed
2. Adding a separate test case for calling updateMetaTags() with no arguments

🔎 Suggested additional test cases

    it('handles empty strings', () => {
      document.title = 'Original';

      updateMetaTags('', '');

      // Empty string is falsy, so title should not change
      expect(document.title).toBe('Original');
+     
+     // Empty string is also falsy for description, so no meta tag should be created
+     const meta = document.querySelector('meta[name="description"]');
+     expect(meta).toBe(null);
    });
+   
+   it('handles no arguments', () => {
+     document.title = 'Original';
+     
+     updateMetaTags();
+     
+     // Nothing should change when no arguments provided
+     expect(document.title).toBe('Original');
+     const meta = document.querySelector('meta[name="description"]');
+     expect(meta).toBe(null);
+   });
  });

frontend/src/components/__tests__/Spinner.test.ts (1)

30-92: Consider parameterized tests to reduce duplication.

The size and color prop tests follow identical patterns. You could use Vitest's it.each() to make these more concise and maintainable:

💡 Example refactor using it.each

  describe('size prop', () => {
-    it('applies small size class', () => {
-      render(Spinner, { props: { size: 'small' } });
-
-      const svg = screen.getByRole('status');
-      expect(svg.classList.contains('h-4')).toBe(true);
-      expect(svg.classList.contains('w-4')).toBe(true);
-    });
-
-    it('applies medium size class by default', () => {
-      render(Spinner);
-
-      const svg = screen.getByRole('status');
-      expect(svg.classList.contains('h-6')).toBe(true);
-      expect(svg.classList.contains('w-6')).toBe(true);
-    });
-
-    it('applies large size class', () => {
-      render(Spinner, { props: { size: 'large' } });
-
-      const svg = screen.getByRole('status');
-      expect(svg.classList.contains('h-8')).toBe(true);
-      expect(svg.classList.contains('w-8')).toBe(true);
-    });
-
-    it('applies xlarge size class', () => {
-      render(Spinner, { props: { size: 'xlarge' } });
-
-      const svg = screen.getByRole('status');
-      expect(svg.classList.contains('h-12')).toBe(true);
-      expect(svg.classList.contains('w-12')).toBe(true);
-    });
+    it.each([
+      { size: 'small', height: 'h-4', width: 'w-4' },
+      { size: 'medium', height: 'h-6', width: 'w-6' },
+      { size: 'large', height: 'h-8', width: 'w-8' },
+      { size: 'xlarge', height: 'h-12', width: 'w-12' },
+    ])('applies $size size class', ({ size, height, width }) => {
+      render(Spinner, { props: { size: size === 'medium' ? undefined : size } });
+      const svg = screen.getByRole('status');
+      expect(svg.classList.contains(height)).toBe(true);
+      expect(svg.classList.contains(width)).toBe(true);
+    });
  });

  describe('color prop', () => {
-    it('applies primary color by default', () => {
-      render(Spinner);
-
-      const svg = screen.getByRole('status');
-      expect(svg.classList.contains('text-primary')).toBe(true);
-    });
-
-    it('applies white color class', () => {
-      render(Spinner, { props: { color: 'white' } });
-
-      const svg = screen.getByRole('status');
-      expect(svg.classList.contains('text-white')).toBe(true);
-    });
-
-    it('applies current color class', () => {
-      render(Spinner, { props: { color: 'current' } });
-
-      const svg = screen.getByRole('status');
-      expect(svg.classList.contains('text-current')).toBe(true);
-    });
-
-    it('applies muted color class', () => {
-      render(Spinner, { props: { color: 'muted' } });
-
-      const svg = screen.getByRole('status');
-      expect(svg.classList.contains('text-gray-400')).toBe(true);
-    });
+    it.each([
+      { color: 'primary', expected: 'text-primary' },
+      { color: 'white', expected: 'text-white' },
+      { color: 'current', expected: 'text-current' },
+      { color: 'muted', expected: 'text-gray-400' },
+    ])('applies $color color class', ({ color, expected }) => {
+      render(Spinner, { props: { color: color === 'primary' ? undefined : color } });
+      const svg = screen.getByRole('status');
+      expect(svg.classList.contains(expected)).toBe(true);
+    });
  });

frontend/src/components/__tests__/ToastContainer.test.ts (2)

127-148: Consider extracting timer-switching pattern.

The test manually switches between fake and real timers. This pattern is repeated in other tests (e.g., line 266-285). Consider extracting this into a helper function or using a dedicated test suite with real timers to improve consistency.

Example approach

describe('close button (with real timers)', () => {
  beforeEach(() => {
    vi.useRealTimers();
    toasts.set([]);
  });
  
  afterEach(() => {
    toasts.set([]);
  });
  
  it('removes toast when close button clicked', async () => {
    // test implementation without timer switching
  });
});

---

209-232: Strengthen progress bar assertion.

The test stores initialTransform (line 221) but never uses it. The assertion only checks that the transform contains 'scaleX', not that the progress actually decreased.

🔎 Proposed improvement

     const alert = screen.getByRole('alert');
     const timer = alert.querySelector('.timer') as HTMLElement;
-    const initialTransform = timer.style.transform;

     // Advance time
     await vi.advanceTimersByTimeAsync(1000);

-    // Check that transform has changed (progress decreased)
     await waitFor(() => {
       const newTransform = timer.style.transform;
-      // Both should have scaleX but with different values
-      expect(newTransform).toContain('scaleX');
+      // Progress should have decreased (scaleX value should be less than 1)
+      const scaleXMatch = newTransform.match(/scaleX\(([\d.]+)\)/);
+      expect(scaleXMatch).toBeTruthy();
+      expect(parseFloat(scaleXMatch![1])).toBeLessThan(1);
     });

docs/testing/frontend-testing.md (1)

36-75: Add language specifier to the fenced code block.

The directory tree structure should have a language specifier for proper rendering and syntax highlighting.

🔎 Proposed fix

-```
+```text
 src/
 ├── stores/
 │   ├── auth.ts

frontend/src/components/__tests__/NotificationCenter.test.ts (1)

24-132: Complex mock setup with type casting workarounds.

The hoisted mock setup with manual store implementations and type casting workarounds ((null as unknown) as ReturnType<...>) is necessary but adds complexity. The pattern is correct for Vitest's hoisting constraints, but consider:

1. The manual store implementation duplicates Svelte store logic—future changes to store behavior might require updates here.
2. The two-phase initialization (hoisted nulls, then vi.fn() assignment) is a known Vitest limitation.

If this pattern is repeated across multiple test files, consider extracting a shared test utility factory for creating mock stores to reduce duplication and maintenance burden.

frontend/src/components/__tests__/Header.test.ts (6)

4-4: Unused imports detected.

tick and flushSync are imported but never used in this test file. Consider removing them to keep the imports clean.

Proposed fix

-import { tick, flushSync } from 'svelte';

---

62-81: Component mock relies on Svelte internals.

This mock structure works for testing but depends on Svelte's internal $$ object shape. Consider adding a comment noting this may need updates if Svelte's internal API changes in future versions.

---

153-166: Consider cleaning up matchMedia mock in afterEach.

The matchMedia mock is set in beforeEach but not restored in afterEach. While this likely doesn't cause issues within this test file, it could affect other test files if they run in the same Vitest worker. For completeness, consider storing and restoring the original matchMedia implementation.

Proposed fix

 describe('Header', () => {
   let originalInnerWidth: number;
+  let originalMatchMedia: typeof window.matchMedia;

   beforeEach(() => {
     // ... existing code ...

     // Store original window width
     originalInnerWidth = window.innerWidth;
+    originalMatchMedia = window.matchMedia;

     // ... rest of beforeEach ...
   });

   afterEach(() => {
     // Restore window width
     Object.defineProperty(window, 'innerWidth', {
       writable: true,
       configurable: true,
       value: originalInnerWidth,
     });
+    
+    // Restore matchMedia
+    Object.defineProperty(window, 'matchMedia', {
+      writable: true,
+      value: originalMatchMedia,
+    });
   });

---

215-246: SVG path assertions are coupled to implementation details.

The tests check specific SVG path data (e.g., 'M12 3v1', 'M20.354'). While functional, these assertions are brittle—any icon library update or SVG optimization could break them. Consider adding data-testid attributes to the icons in the component for more resilient testing.

---

286-294: CSS class selector in test assertion.

The filter !l.closest('.lg\\:hidden') couples the test to specific Tailwind CSS class names. This works but may break if the styling approach changes. Consider using aria-hidden or data-testid attributes to distinguish desktop vs. mobile elements.

---

466-470: Non-null assertion without prior null check.

Using menuButton! assumes the element exists but expect(menuButton).toBeTruthy() on line 468 only checks truthiness, it doesn't guard the click on line 470. If menuButton is null, the assertion would fail with an unclear error from the click, not from the expect. Consider restructuring:

Proposed fix

       const mobileMenuWrapper = container.querySelector('.block.lg\\:hidden');
       const menuButton = mobileMenuWrapper?.querySelector('button');
-      expect(menuButton).toBeTruthy();
-
-      await user.click(menuButton!);
+      expect(menuButton).not.toBeNull();
+      if (!menuButton) throw new Error('Menu button not found');
+      
+      await user.click(menuButton);

Alternatively, use as HTMLButtonElement after the assertion for cleaner TypeScript narrowing.

frontend/src/components/__tests__/ProtectedRoute.test.ts (4)

48-57: Consider resetting initialize and isAuthenticated mocks if used in future tests.

The initialize and isAuthenticated mocks inside AuthInitializer aren't reset in beforeEach. Currently this is fine since these aren't called in tests, but if future tests call and assert on them, add resets to avoid state leakage between tests.

---

142-158: Consider initializing resolveWaitForInit to avoid non-null assertion.

The variable is assigned inside the Promise executor (which runs synchronously), so the ! assertion works. However, initializing it upfront improves clarity and avoids potential issues if the code is refactored:

Suggested improvement

-    it('shows spinner while auth is initializing', async () => {
-      // Keep waitForInit pending
-      let resolveWaitForInit: () => void;
-      mocks.mockWaitForInit.mockImplementation(() => new Promise(resolve => {
-        resolveWaitForInit = () => resolve(true);
-      }));
+    it('shows spinner while auth is initializing', async () => {
+      // Keep waitForInit pending
+      let resolveWaitForInit: () => void = () => {};
+      mocks.mockWaitForInit.mockImplementation(() => new Promise(resolve => {
+        resolveWaitForInit = () => resolve(true);
+      }));
       // ...
-      resolveWaitForInit!();
+      resolveWaitForInit();
     });

---

177-201: Fixed timeout delays can cause test flakiness.

Using setTimeout(resolve, 50) to wait for "something that shouldn't happen" is fragile—it may flake on slow CI or waste time in fast environments. Consider using Vitest's fake timers or increasing the timeout with a comment explaining the rationale:

Alternative with fake timers

it('does not redirect when authenticated', async () => {
  vi.useFakeTimers();
  
  render(ProtectedRoute);

  await waitFor(() => {
    expect(mocks.mockWaitForInit).toHaveBeenCalled();
  });

  // Advance timers to ensure no delayed redirect
  await vi.advanceTimersByTimeAsync(100);

  expect(mocks.mockGoto).not.toHaveBeenCalled();
  
  vi.useRealTimers();
});

---

323-351: Visual state tests are implementation-coupled but acceptable.

Testing for specific CSS classes like .min-h-screen.flex.items-center.justify-center couples tests to Tailwind implementation. This is a reasonable tradeoff for component-level tests, but be aware that styling refactors will require test updates.

Consider adding data-testid attributes for more stable selectors if styling changes frequently.